-
Notifications
You must be signed in to change notification settings - Fork 5
feat(mro): Improve martian_make_mro #498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Include the filename in the error message of martian_make_mro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely better! Were you able to check that the generated mros did not change as a sanity check?
); | ||
ensure!( | ||
rewrite || !filename.exists(), | ||
"File {} exists. Use --rewrite to overwrite it.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--rewrite
is a caller specific detail, but this is valid for all our use cases
3c5e88f
to
6c63611
Compare
This change did a cause change in the MRO files produced by the tests, though it was fine for the non-test code. I've reverted this change for now. I may take another look at it in a separate PR. |
let args: Vec<_> = std::env::args().collect(); | ||
std::path::Path::new(&args[0]) | ||
Path::new(&std::env::args().next().unwrap()) | ||
.canonicalize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this. Cannonicalization makes this function essentially useless in a bazel runfiles tree.
martian_make_mro
current_executable
as fallback inget_generator_name
current_executable
Slack: https://10xgenomics.slack.com/archives/CJT1RB554/p1717443208330939